Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add special className to style boolean cells (checkbox) in Vanilla package #1865

Conversation

andresgutgon
Copy link
Contributor

@andresgutgon andresgutgon commented Jan 10, 2022

What?

Hi I was using the lib and I noticed Vanilla React has the same styles for input type='text' and input type='checkbox'. Would be possible to have different styles? This way is easier to style it

Order

Also noticed when is a checkbox boolean the position is the same as with type=text
Now:

<label>My label</label>
<input type='checkbox' />

Wouldn't be better to put the cell inside the label?

<label>
  <input type='checkbox' />
  <div>My label</div>
</label>

I think is a common practice for checkboxes

Changing the order checkbox can look like this
image

Thanks!

@sdirix
Copy link
Member

sdirix commented Jan 11, 2022

Thanks for the contribution, we'll take a look very soon! ❤️

@andresgutgon
Copy link
Contributor Author

andresgutgon commented Jan 11, 2022

Nice! Thanks @sdirix what do you think about the order of label/input when is a boolean/checkbox?

At the moment only added the class to style the checkbox. But the order remains the same as with input

@sdirix
Copy link
Member

sdirix commented Jan 11, 2022

Nice! Thanks @sdirix what do you think about the order of label/input when is a boolean/checkbox?

At the moment only added the class to style the checkbox. But the order remains the same as with input

Can you point me to some occurrences where the input is defined within a label? I'm not aware of that pattern. The MDN documentation also doesn't showcase it.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you take a look at the failing test cases?

@andresgutgon andresgutgon force-pushed the fix/vanilla-react-styles-for-checkbox branch from 9277648 to ff27e63 Compare January 13, 2022 12:39
@andresgutgon
Copy link
Contributor Author

Can you point me to some occurrences where the input is defined within a label?

Uhmm maybe is just me. I always do checkbox/radio this way. But maybe is not something standard. Feel free to ignore.

@andresgutgon andresgutgon requested a review from sdirix January 13, 2022 12:41
@andresgutgon
Copy link
Contributor Author

I think I fixed the specs, let me know if it works. Thanks!

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.242% when pulling ff27e63 on andresgutgon:fix/vanilla-react-styles-for-checkbox into e9400b6 on eclipsesource:master.

@sdirix sdirix merged commit 5b123ce into eclipsesource:master Jan 13, 2022
@sdirix
Copy link
Member

sdirix commented Jan 13, 2022

Thanks again for the contribution ;)

@andresgutgon andresgutgon deleted the fix/vanilla-react-styles-for-checkbox branch January 13, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants